Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port PROGMEM from 2.5.0 #1374

Merged
merged 3 commits into from
Aug 12, 2019
Merged

Port PROGMEM from 2.5.0 #1374

merged 3 commits into from
Aug 12, 2019

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Nov 27, 2018

Compilation no longer fails when using F() / DEBUG_MSG_P in some specific cases like sensor classes:

/home/builder/.platformio/packages/framework-arduinoespressif8266@1.20300.1/cores/esp8266/pgmspace.h:21:51: error: __c causes a section type conflict with __c
#define PSTR(s) (__extension__({static const char __c[] PROGMEM = (s); &__c[0];}))
^

#1370 as an example of where this did happen in class method

@Valcob
Copy link
Contributor

Valcob commented Nov 27, 2018

I don't think that is a good idea I overcame that by simply declaring all the string in progmem.h and then just reused them across the firmware there is no need to make constants per each file by wrapping in a modified F() with a counter when you can have them all in one place and just reuse across

@mcspr
Copy link
Collaborator Author

mcspr commented Nov 27, 2018

Uniqueness is not a problem for debug messages though, which is what this solves:
dev...mcspr:travis-progmem-test
https://travis-ci.org/mcspr/espurna/jobs/460520688#L526 changes DEBUG_MSG -> DEBUG_MSG_P when SENSOR_DEBUG is enabled and builds ok
https://travis-ci.org/mcspr/espurna/jobs/460524020#L546 same as above, but without the unique section attr

edit: links

@Valcob
Copy link
Contributor

Valcob commented Nov 28, 2018

@mcspr for the first link the warnings are telling you that comparing a const == a & b is not safe and you should use const == (a & b) if that was what caught you attention
for the second it's a legit thing when you are using F() in classes and that is solved the way I've described above

@xoseperez
Copy link
Owner

@Valcob your suggestion is to move all progmem strings in the code to a separate file? It makes sense to save size for those strings used multiple times and there are some of those. But it also requires some major refactoring.

In the meantime @mcspr changes go in the line of the Arduino Core for ESP8266 project. I would add a check to only apply them if < 2.5.0

Just for reference:
Change included here: esp8266/Arduino#5048
Current code here: https://github.com/esp8266/Arduino/blob/master/tools/sdk/libc/xtensa-lx106-elf/include/sys/pgmspace.h

@mcspr
Copy link
Collaborator Author

mcspr commented Dec 9, 2018

Added comments + defguards.

Have been wondering about PSTR comment from the link above. Testing size, can be removed if it is noticeable.

@mcspr mcspr mentioned this pull request Jun 12, 2019
@ruimarinho
Copy link
Contributor

@mcspr is there any alternative for debugging inside sensor classes while this isn't merged?

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 7, 2019

DEBUG_MSG should work ok (as all SENSOR_DEBUG parts are using it)
Does this patch work ok though?

@mcspr mcspr merged commit f4e3ced into xoseperez:dev Aug 12, 2019
@mcspr mcspr deleted the progmem-port branch August 12, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants